feat: support bundling the raw Pages `_worker.js` before deploying #2404

petebacondarwin · 2022-12-13T17:31:37Z

What this PR solves / how to test:

Adds support to wrangler pages to pass the raw _worker.js file through the normal Wrangler bundler,
which improves support for D1 bindings with web frameworks that generate this file, such as SvelteKit.

You can test whether this is working by creating a Pages project with a _worker.js that imports from another
JS file. E.g.

_worker.js

import value from "./dep";

export default {
  async fetch() {
    return new Response(value);
  },
};

dep.js

export default "TEST";

Associated docs issues/PR:

TBD

Author has included the following, where applicable:

Tests
Changeset

Reviewer has performed the following, where applicable:

Checked for inclusion of relevant tests
Checked for inclusion of a relevant changeset
Checked for creation of associated docs updates
Manually pulled down the changes and spot-tested

FIxes #2153

changeset-bot · 2022-12-13T17:31:41Z

🦋 Changeset detected

Latest commit: ff4f52c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2022-12-13T17:33:42Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/runs/3901445415/npm-package-wrangler-2404

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/prs/2404/npm-package-wrangler-2404

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/runs/3901445415/npm-package-wrangler-2404 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/runs/3901445415/npm-package-cloudflare-pages-shared-2404

penalosa · 2022-12-13T17:36:06Z

This could definitely do with some docs changes, maybe as an addendum to https://developers.cloudflare.com/workers/wrangler/bundling/? Are the wrangler pages ... commands documented anywhere?

codecov · 2022-12-13T17:40:33Z

Codecov Report

Merging #2404 (ff4f52c) into main (fe8c691) will increase coverage by 0.02%.
The diff coverage is 43.28%.

@@            Coverage Diff             @@
##             main    #2404      +/-   ##
==========================================
+ Coverage   72.97%   72.99%   +0.02%     
==========================================
  Files         156      156              
  Lines        9709     9735      +26     
  Branches     2556     2565       +9     
==========================================
+ Hits         7085     7106      +21     
- Misses       2624     2629       +5

Impacted Files	Coverage Δ
packages/wrangler/src/pages/dev.ts	`19.02% <5.55%> (ø)`
...ckages/wrangler/src/pages/functions/buildWorker.ts	`44.59% <52.94%> (+0.59%)`	⬆️
packages/wrangler/src/pages/publish.tsx	`65.36% <64.28%> (-0.71%)`	⬇️
...ckages/wrangler/src/pages/functions/buildPlugin.ts	`20.00% <100.00%> (+7.17%)`	⬆️
packages/wrangler/src/git-client.ts	`81.25% <0.00%> (+4.16%)`	⬆️
...ackages/wrangler/src/__tests__/helpers/mock-bin.ts	`100.00% <0.00%> (+5.26%)`	⬆️

petebacondarwin · 2022-12-13T17:44:27Z

By the way, I found that quite often the "integration" tests failed simply because they overran the default 5 seconds. It was completely non-deterministic and depended upon the workload on the machine and other random factors. I bumped up the timeouts on all the ones that tended to flake out to 10 secs.

fixtures/external-durable-objects-app/tests/index.test.ts

fixtures/node-app-pages/tests/index.test.ts

fixtures/pages-functions-with-routes-app/tests/index.test.ts

fixtures/pages-workerjs-and-functions-app/tests/index.test.ts

packages/wrangler/src/pages/dev.tsx

packages/wrangler/src/pages/functions/buildWorker.ts

packages/wrangler/src/pages/publish.tsx

GregBrimble · 2022-12-14T10:02:48Z

Pete, you're my hero. I'll try review this today.

fixtures/node-app-pages/tests/index.test.ts

fixtures/pages-workerjs-app/tests/index.test.ts

fixtures/pages-workerjs-with-routes-app/tests/index.test.ts

fixtures/pages-workerjs-and-functions-app/tests/index.test.ts

GregBrimble · 2022-12-14T18:55:58Z

packages/wrangler/src/pages/dev.tsx

@@ -78,6 +79,11 @@ export function Options(yargs: Argv) {
 				description:
 					"The location of the single Worker script if not using functions",
 			},
+			"bundle-worker": {


We will make this default at some point pretty soon. So I'm tempted to make this an env var or deprecated/experimental or whatever so we can remove it easily when we force it. What do you think, @petebacondarwin ?

Actually, to keep in line with wrangler publish we should keep this and call it bundle. The usage in wrangler publish is that this defaults to true but can be turned off with no-bundle or bundle=false.

So I would rather not hide it away - I think there may be valid cases where people choose not to run the bundler on the generated _worker.js.

GregBrimble · 2022-12-14T19:06:01Z

.changeset/tiny-knives-compare.md

+command line argument to `wrangler pages dev` and `wrangler pages publish`. For
+backward compatibility this flag defaults to `false` if not provided.


Suggested change

command line argument to `wrangler pages dev` and `wrangler pages publish`. For

backward compatibility this flag defaults to `false` if not provided.

command line argument to `wrangler pages dev` and `wrangler pages publish`.

If you're happy, I'd like to drop that line from the release notes since I will be doing this by default soon enough.

GregBrimble · 2022-12-14T19:10:29Z

packages/wrangler/src/pages/publish.tsx

+		let workerFileContents = _workerJS;
+		if (bundleWorker) {
+			const outfile = join(tmpdir(), `./bundledWorker-${Math.random()}.mjs`);
+			await buildRawWorker({


Do we not need to listen for onEnd to complete here and in the checkWorker() call?

I think onEnd() is only really helpful when watch: true, right?
Without watching the call to buildRawWorker() will only resolve after the build completes.

For watch mode, the returned promise resolves after the first build but then we need a way to be notified after subsequent builds.

May be fixed by cloudflare/workers-sdk#2404.

JacobMGEvans

I'm guessing based on the nature of these changes it had to be little beefy PR. I would appreciate some of your time for a pair review on this one.

petebacondarwin · 2023-01-11T07:25:02Z

I'm guessing based on the nature of these changes it had to be little beefy PR. I would appreciate some of your time for a pair review on this one.

This is not as beefy as it looks. But that is my fault because I have wrapped a bunch of test changes into the PR that are not actually necessary for this PR. At the time of writing I had to make these changes to get the flaky fixture tests to pass, but I think with switching them to Vitest they probably are less flaky now.

So you can mostly ignore the changes to all the fixtures/**/*.test.ts files on a first read.

After that, there is the addition of the --bundle arg, which gets passed down the line to the bundling code.
And finally there is some clean up of where the pages functions actually need to bundle stuff, which deduplicates some esbuild plugins into shared functions.

petebacondarwin · 2023-01-11T07:47:45Z

I have split the PR into two commits to make it easier to see the main change from the test refactor.

Skye-31 · 2023-01-11T09:35:46Z

@petebacondarwin
Could we consider warning/providing an error for the user when bundle: false && d1Databases.length in pages dev?

petebacondarwin · 2023-01-11T09:47:30Z

@petebacondarwin Could we consider warning/providing an error for the user when bundle: false && d1Databases.length in pages dev?

I would be happy with that. But in a follow up PR.

packages/wrangler/src/pages/dev.ts

packages/wrangler/src/pages/functions/buildWorker.ts

packages/wrangler/src/pages/publish.tsx

CarmenPopoviciu · 2023-01-11T18:06:05Z

LGTM! Only those 3 wording nits and this is good to go AFAIAC 🎉

JacobMGEvans · 2023-01-11T18:34:17Z

I'm guessing based on the nature of these changes it had to be little beefy PR. I would appreciate some of your time for a pair review on this one.

This is not as beefy as it looks. But that is my fault because I have wrapped a bunch of test changes into the PR that are not actually necessary for this PR. At the time of writing I had to make these changes to get the flaky fixture tests to pass, but I think with switching them to Vitest they probably are less flaky now.

So you can mostly ignore the changes to all the fixtures/**/*.test.ts files on a first read.

After that, there is the addition of the --bundle arg, which gets passed down the line to the bundling code. And finally there is some clean up of where the pages functions actually need to bundle stuff, which deduplicates some esbuild plugins into shared functions.

Thanks for the explanation. Considering the fixtures, there is a lot less to the PR.

JacobMGEvans

PR is already approved. Nothing I mentioned is blocking.

JacobMGEvans · 2023-01-11T18:36:08Z

packages/wrangler/src/pages/dev.ts

+		let runBuild = async () => {
+			await checkRawWorker(workerScriptPath, () => scriptReadyResolve());


Codecov is mentioning these lines are uncovered by tests? Is this covered in the fixtures rather than the wrangler package tests (Codecov doesnt collect fixtures tests I think)

Yeah this is covered by fixtures tests.

JacobMGEvans · 2023-01-11T18:38:19Z

packages/wrangler/src/pages/dev.ts

@@ -278,47 +295,39 @@ export const Handler = async ({
 		});
 	} else if (usingFunctions) {
 		// Try to use Functions
-		const outfile = join(tmpdir(), `./functionsWorker-${Math.random()}.mjs`);
-		scriptPath = outfile;
+		scriptPath = join(tmpdir(), `./functionsWorker-${Math.random()}.mjs`);


Not part of your PR, but wouldnt it make more sense to have this idempotent by hashing someting dynamic and unique to the functionsWorker?

It doesn't really matter what the name is as long as it is unlikely to collide with another concurrent build. This was the simplest solution.

We now use helper functions to setup and teardown a long-lived wrangler process. This means that we can remove the `--forceExit` option from the jest setup.

Previously, if you provided a `_worker.js` file, then Pages would simply check the file for disallowed imports and then deploy the file as-is. Not bundling the `_worker.js` file means that it cannot containing imports to other JS files, but also prevents Wrangler from adding shims such as the one for the D1 alpha release. This change adds the ability to tell Wrangler to pass the `_worker.js` through the normal Wrangler bundling process before deploying by setting the `--bundle` command line argument to `wrangler pages dev` and `wrangler pages publish`. This is in keeping with the same flag for `wrangler publish`. Currently bundling is opt-in, flag defaults to `false` if not provided.

…ying

lrapoport-cf · 2023-01-12T18:06:08Z

This could definitely do with some docs changes, maybe as an addendum to https://developers.cloudflare.com/workers/wrangler/bundling/? Are the wrangler pages ... commands documented anywhere?

@petebacondarwin is there a docs PR that should be linked to this?

petebacondarwin · 2023-01-13T08:21:38Z

This could definitely do with some docs changes, maybe as an addendum to https://developers.cloudflare.com/workers/wrangler/bundling/? Are the wrangler pages ... commands documented anywhere?

@petebacondarwin is there a docs PR that should be linked to this?

Ah sorry. I missed this comment.
The pages commands are documented here: https://developers.cloudflare.com/workers/wrangler/commands/#pages
I will create a PR to update them.

petebacondarwin requested review from a team as code owners December 13, 2022 17:31

petebacondarwin requested a review from GregBrimble December 13, 2022 17:42

penalosa reviewed Dec 13, 2022

View reviewed changes

Skye-31 reviewed Dec 14, 2022

View reviewed changes

packages/wrangler/src/pages/functions/buildWorker.ts Outdated Show resolved Hide resolved

Skye-31 reviewed Dec 14, 2022

View reviewed changes

packages/wrangler/src/pages/publish.tsx Show resolved Hide resolved